Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Adding "style-src 'unsafe-inline' 'self'" to default CSP rules #41305

Merged
merged 12 commits into from
Aug 9, 2019
Merged

Adding "style-src 'unsafe-inline' 'self'" to default CSP rules #41305

merged 12 commits into from
Aug 9, 2019

Conversation

kobelb
Copy link
Contributor

@kobelb kobelb commented Jul 16, 2019 •
edited
Loading

Prior to this change, we didn't have any rules that applied to style elements. This change allows us to use unsafe-inline styles, and styles that are loaded from "self".

DevDocs

Adding style-src 'unsafe-inline' 'self' to the default CSP rules

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb kobelb marked this pull request as ready for review July 26, 2019 14:28
@kobelb kobelb requested a review from a team as a code owner July 26, 2019 14:28
@kobelb kobelb added release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0 and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Jul 26, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb kobelb requested a review from azasypkin August 5, 2019 14:43
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

azasypkin
azasypkin approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested locally (dashboards, visualizations, canvas, siem etc.) and haven't noticed anything that is broken because of this change. Just left one question and note

src/legacy/server/csp/index.ts
@@ -26,6 +26,7 @@ export const DEFAULT_CSP_RULES = Object.freeze([
`script-src 'unsafe-eval' 'nonce-{nonce}'`,
'worker-src blob:',
'child-src blob:',
`style-src 'unsafe-inline' 'self'`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is there any reason we don't want to use nonce for inline styles as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the nonce has caused quite a few issues, as discussed here and I'd like to find a way to move toward self even for script-src. I could see the argument being made for using the nonce until we switch script-src to self, but it's quite a bit more work which we'd rather quickly get rid of.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see, thanks for the explanation. Yeah, let's see how it goes then.

test/api_integration/apis/general/csp.js Outdated

const scriptSrc = parsed.get('script-src');
expect(scriptSrc).to.be.an(Array);
expect(scriptSrc).not.to.contain('unsafe-inline');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: I'm a bit concerned about check like this. Our parsing logic in this test may not work as we expect at some point (even with packages like content-security-policy-parser) and we'll get false negatives here. Is there any reason we don't want to use expect().to.eql with a full list of "directives" here? Is it because of the nonce-some-unguessable-value? If so, can we filter startsWith('nonce-') out before asserting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I initially was trying to keep this test similar to how it was originally written, but you've made a convincing argument. Changes will be forth-coming.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@kobelb
Copy link
Contributor Author

kobelb commented Aug 9, 2019

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@kobelb kobelb merged commit 2ce2bfb into elastic:master Aug 9, 2019
@kobelb kobelb deleted the csp/style-src branch August 9, 2019 22:15
@kobelb kobelb mentioned this pull request Aug 9, 2019
Merged
kobelb added a commit to kobelb/kibana that referenced this pull request Aug 9, 2019
@kobelb
Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (elast…
2219041 
…ic#41305)

* Adding "style-src 'unsafe-inline' 'self'" to default CSP rules

* Updating jest snapshot

* Fixing api integration smoke test

* Verifying all CSP responses

* Fixing OIDC implicit flow test
jloleysens added a commit to jloleysens/kibana that referenced this pull request Aug 12, 2019
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into feature/rollu…
2c3617d 
…p-metrics-selectall

* 'master' of github.com:elastic/kibana: (22 commits)
  [Code]: downgrade the log level of error message from subprocess (elastic#42925)
  [Code] Cancel clone/update job in the middle if disk space over the watermark (elastic#42890)
  Add Kibana App specific URL to the help menu (elastic#34739) (elastic#42580)
  [Maps] refactor createShapeFilterWithMeta to support more than just polygons (elastic#43042)
  Skip flaky es_ui_shared/request tests.
  Pass uiSettings to all data plugin services (elastic#42159)
  [SIEM] Upgrades react-redux and utilize React.memo for performance gains  (elastic#43029)
  [skip-ci][Maps] add maki icon sheet to docs (elastic#43063)
  Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (elastic#41305)
  Update dependency commander to v3 (elastic#43041)
  Update dependency @percy/agent to ^0.10.0 (elastic#40517)
  [Maps] only show top hits checkbox if index has date fields (elastic#43056)
  run chained_controls on Firefox to catch regression (elastic#43044)
  fixing issue with dashboard csv download (elastic#42964)
  Expose task manager as plugin instead of server argument (elastic#42966)
  Expose createRouter from HttpService, prepare handlers for context introduction (elastic#42686)
  [Code] disk watermark supports percentage and absolute modes (elastic#42987)
  [apps/dashboard] skip part of filtering tests on FF (elastic#43047)
  [ML] Kibana management jobs list (elastic#42570)
  [ML] Fix check for watcher being enabled (elastic#43025)
  ...
kobelb added a commit that referenced this pull request Aug 12, 2019
@kobelb
Adding "style-src 'unsafe-inline' 'self'" to default CSP rules (#41305)…
c53e75d 
… (#43065)

* Adding "style-src 'unsafe-inline' 'self'" to default CSP rules

* Updating jest snapshot

* Fixing api integration smoke test

* Verifying all CSP responses

* Fixing OIDC implicit flow test
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azasypkin azasypkin azasypkin approved these changes

Assignees
No one assigned
Labels
release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. v7.4.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kobelb @elasticmachine @azasypkin